Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactoring pii_redactor as its own dpk_ named module #895

Merged
merged 13 commits into from
Jan 16, 2025

Conversation

matouma
Copy link
Contributor

@matouma matouma commented Dec 19, 2024

Why are these changes needed?

First pass at restructuring the transform built as its own module (e.g. dpk_pii_redactor) with a ray submodule (dpk_pii_redactor.ray ).

Removed python and ray folders and refactor Dockerfile.python and Dockerfile.ray
remove pyproject.toml and Makefiles
move python code under dpk_pii_redactor
move ray code under dpk_pii_redactor/ray
change import statement to include module name
replace recursive Makefile and use targets from .make.cicd.targets
adapt kfp_ray/Makefile and other make target

Related issue number (if any).

#774

@touma-I
Copy link
Collaborator

touma-I commented Dec 19, 2024

@SowmyaLR We are in the process of refactoring all the transforms code as named modules. Please take a look at how I did your code and provide comments/feedback/approval on the proposed changes. The intent is to simplify the structure so Transforms developer have fewer things to deal with and also so all the transforms can be packaged together as a single pypi. thanks

touma-I and others added 5 commits December 19, 2024 12:04
Signed-off-by: Maroun Touma <[email protected]>
Signed-off-by: Maroun Touma <[email protected]>
Signed-off-by: Maroun Touma <[email protected]>
Signed-off-by: SHAHROKH DAIJAVAD <[email protected]>
@touma-I touma-I marked this pull request as ready for review December 23, 2024 14:06
@touma-I touma-I self-requested a review December 23, 2024 14:06
@touma-I
Copy link
Collaborator

touma-I commented Jan 6, 2025

@SowmyaLR Hope all is well. Are you in piosition to review, approve or provide comments on this PR ? Thanks

@SowmyaLR
Copy link
Collaborator

SowmyaLR commented Jan 6, 2025

@SowmyaLR Hope all is well. Are you in piosition to review, approve or provide comments on this PR ? Thanks

Hi @touma-I

Thank you for letting me know. I can review the code this weekend I hope that can work out.

@touma-I
Copy link
Collaborator

touma-I commented Jan 6, 2025

@SowmyaLR Hope all is well. Are you in piosition to review, approve or provide comments on this PR ? Thanks

Hi @touma-I

Thank you for letting me know. I can review the code this weekend I hope that can work out.

Great. thanks @SowmyaLR

@shahrokhDaijavad
Copy link
Member

Hi, @SowmyaLR When will you be able to review the restructured code? We are anxious to merge the new code after your review.

@SowmyaLR
Copy link
Collaborator

LGTM, in all the source files only the import is changed.

@SowmyaLR
Copy link
Collaborator

Thank you everyone for making this change @shahrokhDaijavad @touma-I @matouma

@touma-I
Copy link
Collaborator

touma-I commented Jan 16, 2025

LGTM, in all the source files only the import is changed.

That is correct. This is needed so each transform is in its own named module.

Signed-off-by: Maroun Touma <[email protected]>
Signed-off-by: Maroun Touma <[email protected]>
Signed-off-by: Maroun Touma <[email protected]>
Signed-off-by: Maroun Touma <[email protected]>
Signed-off-by: Maroun Touma <[email protected]>
Copy link
Member

@shahrokhDaijavad shahrokhDaijavad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@touma-I touma-I merged commit 725fdf6 into IBM:dev Jan 16, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants